Update generators to add fallback: { stream: false } to webpack configs
#1676
Conversation
- Set fallback for 'stream' to false in both client and server Webpack configurations. - Cleaned up package.json by removing unnecessary stream-browserify alias. - Update generators to add the fallback statement to webpackConfig.js These changes enhance compatibility and streamline the build process.
|
Warning Rate limit exceeded@AbanoubGhadban has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 20 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe pull request introduces modifications to Webpack configurations across multiple files, focusing on handling the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/generators/react_on_rails/templates/base/base/config/webpack/webpackConfig.js.tt (1)
9-11: LGTM! Consider clarifying the conditional comment.The fallback configuration for stream is correctly set to false for both client and server configs. However, the comment about removing the server config line could be more specific.
Consider updating the comment to be more explicit:
- // Remove the next line if you used "node" target in serverWebpackConfig.js + // Remove the serverConfig.resolve.fallback line if you set target: "node" in serverWebpackConfig.js + // as Node.js already provides the stream module
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/generators/react_on_rails/templates/base/base/config/webpack/webpackConfig.js.tt(1 hunks)spec/dummy/config/webpack/alias.js(0 hunks)spec/dummy/config/webpack/webpackConfig.js(1 hunks)spec/dummy/package.json(1 hunks)
💤 Files with no reviewable changes (1)
- spec/dummy/config/webpack/alias.js
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: examples (newest)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: build-dummy-app-webpack-test-bundles (oldest)
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: build
- GitHub Check: examples (oldest)
🔇 Additional comments (2)
spec/dummy/config/webpack/webpackConfig.js (1)
7-9: Verify the fallback configuration in the dummy app.The fallback configuration matches the template. Let's verify if any tests depend on stream functionality.
✅ Verification successful
Stream fallback configuration is correctly set
The
stream: falsefallback in webpack configuration is safe as the codebase only uses Rails Turbo Streams feature and doesn't depend on Node.js stream module.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for stream-related tests or implementations in the dummy app rg -g '!node_modules' -g '!*.lock' -g '!package.json' 'stream' spec/dummy/Length of output: 1137
spec/dummy/package.json (1)
80-80: Verify if removing --watch was intentional.The
--watchflag was removed from thebuild:devscript while other watch-related scripts remain unchanged. This change appears unrelated to the stream fallback configuration updates.Could you confirm if removing the watch flag was intentional? If not, consider keeping it consistent with other dev scripts:
- "build:dev": "rm -rf public/webpack/development && yarn build:rescript && RAILS_ENV=development NODE_ENV=development bin/shakapacker", + "build:dev": "rm -rf public/webpack/development && yarn build:rescript && RAILS_ENV=development NODE_ENV=development bin/shakapacker --watch",✅ Verification successful
The removal of --watch flag appears intentional and correct
The package.json shows a clear pattern where watch functionality is deliberately separated into dedicated scripts (build:dev:server and build:dev:watch), while regular build scripts like build:test and build:dev are meant for one-time builds.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # View the complete package.json to understand build script patterns cat spec/dummy/package.json # Search for any related build script changes in commit messages or PR gh pr view --json title,bodyLength of output: 3309
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.md (1)
870-870: Add language specifier to the code block.Add 'ruby' as the language specifier to the fenced code block for proper syntax highlighting.
-``` +```ruby # This configures the script to run to build the production assets by webpack. Set this to nil # if you don't want react_on_rails building this file for you. config.build_production_command = "npm run build:production"🧰 Tools
🪛 Markdownlint (0.37.0)
870-870: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.md(2 hunks)
🧰 Additional context used
🪛 Markdownlint (0.37.0)
CHANGELOG.md
870-870: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: examples (newest)
- GitHub Check: build-dummy-app-webpack-test-bundles (oldest)
- GitHub Check: build
- GitHub Check: examples (oldest)
- GitHub Check: rspec-package-tests (oldest)
🔇 Additional comments (1)
CHANGELOG.md (1)
21-23: LGTM! Well-structured changelog entry.The entry follows best practices by clearly describing the change, including the PR reference, and properly attributing the author.
lib/generators/react_on_rails/templates/base/base/config/webpack/webpackConfig.js.tt
Show resolved
Hide resolved
|
Superseded by #1680 |
Judahmeek
deleted the
abanoubghadban/pro1674/fix-cant-resolve-stream-while-bundling
branch
These changes enhance compatibility and streamline the build process.
Summary
Remove this paragraph and provide a general description of the code changes in your pull
request... were there any bugs you had fixed? If so, mention them. If
these bugs have open GitHub issues, be sure to tag them here as well,
to keep the conversation linked together.
Pull Request checklist
Remove this line after checking all the items here. If the item is not applicable to the PR, both check it out and wrap it by
~.Add the CHANGELOG entry at the top of the file.
Other Information
Remove this paragraph and mention any other important and relevant information such as benchmarks.
This change is
Summary by CodeRabbit
Configuration Changes
streammodule by setting fallback tofalse.stream-browserifyalias from Webpack configuration.Build Process
package.json.Changelog Updates